Skip to content

fix: helm default for etcd PodDisruptionBudget #4143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: helm default for etcd PodDisruptionBudget #4143

wants to merge 1 commit into from
+10 −0

Conversation

@tmonty12
Copy link
Contributor

@tmonty12 tmonty12 commented Nov 6, 2025 •
edited by coderabbitai bot
Loading

Overview:

Our default values.yaml for the the dynamo platform installation has 1 etcd replica. The etcd helm chart creates a PodDisruptionBudget with minAvailable 51%. Node draining fails because it can't evict the etcd pod given the PDB. This sets the default minAvailable on the etcd PDB to 0

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Chores
    • Enhanced platform infrastructure resilience through configuration that improves pod availability management during system maintenance and cluster upgrades, enabling better control over rolling updates and reducing potential service disruptions during maintenance windows.

@tmonty12 tmonty12 requested a review from a team as a code owner November 6, 2025 01:09
@github-actions github-actions bot added the fix label Nov 6, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 6, 2025

Walkthrough

Pod Disruption Budget configuration is added to the etcd section of the Helm platform values file. The new pdb block includes creation flag, minimum availability, and maximum unavailability settings, with accompanying documentation comments explaining behavior during node draining and single-replica scenarios.

Changes

Cohort / File(s) Change Summary
Helm Configuration
deploy/cloud/helm/platform/values.yaml		 Adds new etcd.pdb configuration block with create, minAvailable, and maxUnavailable keys, plus documentation comments on rolling update behavior.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Configuration addition only; no logic or behavioral changes to existing code
  • Single file with straightforward, well-commented configuration keys
  • Verify PDB defaults (create: true, minAvailable: 0, maxUnavailable: "") align with etcd operational requirements

Poem

🐰 A new Pod Disruption Budget hops into place,
Protecting etcd with grace and care,
When nodes drain and replicas bear the trace,
Our drains will pause with thoughtful flair!

Pre-merge checks

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive While the Overview section is detailed and explains the problem well, the Details and Where should reviewer start sections are empty or incomplete, and the Related Issues section contains only a placeholder. Complete the Details section with specific changes made, specify which files to review closely, and replace the GitHub issue placeholder with the actual issue number.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: setting the Helm default for etcd's PodDisruptionBudget, which directly addresses the node draining issue mentioned in the PR.
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25fc732 and 4d9c80c.

📒 Files selected for processing (1)
  • deploy/cloud/helm/platform/values.yaml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: trtllm (amd64)
  • GitHub Check: vllm (amd64)
  • GitHub Check: operator (arm64)
  • GitHub Check: sglang (amd64)
  • GitHub Check: sglang (arm64)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
deploy/cloud/helm/platform/values.yaml (1)

203-211: ****

The PDB configuration is valid and follows bitnami's officially supported pattern. The bitnami etcd Helm chart includes PodDisruptionBudget support with parameters for pdb.create, pdb.minAvailable, and pdb.maxUnavailable. The bitnami chart's own defaults include both minAvailable: 51% and maxUnavailable: "", which is the same pattern used in this PR—only with minAvailable set to 0 for the single-replica scenario. The template properly handles empty string values for optional fields, so there is no mutual exclusivity conflict or rendering error risk. The configuration is correct and ready.

Likely an incorrect or invalid review comment.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tmonty12 tmonty12 enabled auto-merge (squash) November 6, 2025 16:36
@tmonty12 tmonty12 force-pushed the tmonty12/etcd-values-pdb branch from 4d9c80c to 0dae050 Compare November 6, 2025 16:54
@tmonty12 tmonty12 disabled auto-merge November 6, 2025 19:20
@tmonty12 tmonty12 force-pushed the tmonty12/etcd-values-pdb branch from 0dae050 to 31e0306 Compare November 6, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julienmancuso julienmancuso julienmancuso approved these changes

Assignees

No one assigned

Labels

fix size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tmonty12 @julienmancuso